Skip to content

Comments

feat: Add WithFxOption#2956

Merged
MarcoPolo merged 2 commits intomasterfrom
marco/with-fx-option
Oct 22, 2024
Merged

feat: Add WithFxOption#2956
MarcoPolo merged 2 commits intomasterfrom
marco/with-fx-option

Conversation

@MarcoPolo
Copy link
Collaborator

@MarcoPolo MarcoPolo commented Sep 10, 2024

While doing some refactoring of the p2p-forge client, I found that I wanted a way to get access to parts of the host earlier than .New() returning. I also wanted an option I could provide the constructor that would give me the host (or peer.ID, or something else) without having to have the user explicitly pass it to me.

In other words I think it's nicer to do this:

// someServiceThatDependsOnHostAndConfiguresHost
service := NewServiceThatDependsOnHostAndConfiguresHost()
h, err := New(service.Libp2pOptions())

Rather than

// someServiceThatDependsOnHostAndConfiguresHost
service := NewServiceThatDependsOnHostAndConfiguresHost()
h, err := New(service.Libp2pOptions())
service.ProvideHost(h)

And this change would enable the former (along with many other things as well).

The biggest downside here is that fx is really powerful and this could cause some confusing errors to users.

Another concern is that it ties us to fx as long as we support this option. But I'm not worried about this for two reasons:

  1. Fx is pretty nice, and as long as it doesn't break, I don't see a reason to move away from it.
  2. I've marked it as experimental so we can feel less bad about break this.

@Wondertan
Copy link
Contributor

Wondertan commented Sep 10, 2024

Another option would be to provide libp2p options to users who rely on FX as well, but instead of libp2p controlling the fx.New and the user supplying FX options to libp2p, the libp2p provides itself as an option that users can import and inject into their fx.New and dep graph.

@MarcoPolo
Copy link
Collaborator Author

MarcoPolo commented Sep 10, 2024

Another option would be to provide libp2p options to users who rely on FX as well, but instead of libp2p controlling the fx.New and the user supplying FX options to libp2p, the libp2p provides itself as an option that users can import and inject into their fx.New and dep graph.

Yes. I'm working on this as well. It will appear as effectively a new constructor. This option allows hooking into the existing constructor. Long term, providing FX options is a lot more modular and where I want to end up.

Would Celestia be interested in trying that API? Useful for me to gauge interest here :)

@MarcoPolo MarcoPolo force-pushed the marco/with-fx-option branch from 9f2dd3f to 6cd7fa6 Compare October 8, 2024 23:59
@MarcoPolo MarcoPolo mentioned this pull request Oct 9, 2024
Closed
@MarcoPolo MarcoPolo force-pushed the marco/with-fx-option branch from 6cd7fa6 to 0e76f3d Compare October 22, 2024 17:44
This was referenced Oct 22, 2024
Closed
Merged
Closed
@MarcoPolo MarcoPolo merged commit c4c3a34 into master Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@MarcoPolo @Wondertan